-
-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure CME does not happen when (un)registering commands on Paper during server runtime #501
base: dev/dev
Are you sure you want to change the base?
Conversation
...i-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/PaperImplementations.java
Outdated
Show resolved
Hide resolved
...i-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/PaperImplementations.java
Outdated
Show resolved
Hide resolved
(For what it's worth, my multi-threaded programming knowledge is very limited, so I have no comments to make about the implementation from this PR) |
Ah, I think I figured out why the pool only uses one thread. From the javadocs:
So, looking at Paper's public static final java.util.concurrent.ThreadPoolExecutor COMMAND_SENDING_POOL = new java.util.concurrent.ThreadPoolExecutor(
0, 2, 60L, java.util.concurrent.TimeUnit.SECONDS,
new java.util.concurrent.LinkedBlockingQueue<>(),
new com.google.common.util.concurrent.ThreadFactoryBuilder()
.setNameFormat("Paper Async Command Builder Thread Pool - %1$d")
.setUncaughtExceptionHandler(new net.minecraft.DefaultUncaughtExceptionHandlerWithName(net.minecraft.server.MinecraftServer.LOGGER))
.build(),
new java.util.concurrent.ThreadPoolExecutor.DiscardPolicy()
); They're using an unbounded So, that means this PR should work. Submitting the CommandAPI's register and unregister tasks to the thread pool will ensure that no CMEs happen. The pool only uses one thread at a time, so its either reading from the dispatchers to make a Commands packet or modifying the dispatchers as the CommandAPI requests. However, this might be relying on unintended behavior. Given that Anyway, the implementation for |
Hm, unfortunately, this fix does not solve the problem on all versions of Paper. The Specifically, the original PR (PaperMC/Paper#3116, build paper-1.15.2-177) submits its task to the common java.util.concurrent.ForkJoinPool.commonPool().execute(() -> {
sendAsync(entityplayer);
}); A random commit later on (PaperMC/Paper@7323594, build paper-1.18-31) changed this to use some sort of net.minecraft.server.MCUtil.scheduleAsyncTask(() -> this.sendAsync(player)); For reference, the current system (PaperMC/Paper#8170, build paper-1.19.2-117) changed it to this: COMMAND_SENDING_POOL.execute(() -> {
this.sendAsync(player);
}); So, there needs to be more work to fix this on older Paper versions. |
Commands.COMMAND_SENDING_POOL
on Paper8ce0a6e
to
c457c8c
Compare
Alright, I basically rewrote this whole thing with the new research in mind. Now, the CommandAPI intercepts I haven't tested this yet, but I think it should work. It also shouldn't interfere with anything on Spigot. TODO:Test on Spigot
Test on Paper
|
…n older versions
Hmm, the current solution doesn't quite work. For versions ~1.15 to 1.18 java.lang.reflect.InaccessibleObjectException:
Unable to make field static final java.util.concurrent.ForkJoinPool
java.util.concurrent.ForkJoinPool.common accessible:
module java.base does not "opens java.util.concurrent" to unnamed module @14aca5ab I'll need to find something else for this again |
On pause until #517. That PR splits up the Spigot and Paper modules, which will be helpful since this PR won't have to do separate Paper detection that works across all versions. |
This PR fixes #494
PaperMC/Paper#3116 added a patch to Paper that causes the Commands packet to be built asynchronously. Specifically, they added the following
ThreadPoolExector
to thenet.minecraft.commands.Commands
class and used it to move some of the work off the main thread.Previously, if this thread pool happened to be looping through the Brigadier Vanilla or Resources CommandDispatcher and the CommandAPI registered or unregistered a command, a
ConcurrentModificationException
could occur.This PR fixes this by adding a
modifyCommandTreesSafely
method toPaperImplementations
. Anytime CommandAPIBukkit needs to modify the command trees, it calls this method. If theCOMMAND_SENDING_POOL
is present, the modification task will be submitted to the pool, and the current thread will be blocked until the task is completed. If the thread pool is not present, the task is run immediately like before.I chose this solution because I noticed that the
COMMAND_SENDING_POOL
seemed to only run one task at a time. So, if you put the modify task into the pool, you can ensure that the pool isn't running another task, and no CME occurs.However, the fact that the pool only ran one task at a time struck me as odd. The javadocs for the
ThreadPoolExecutor
constructor indicate the second parameter —2
— is the maximum number of threads allowed in the pool. Therefore, I expected that at most 2 tasks could be run at the same time in this pool, potentially allowing a modify task and a command build task to run at the same time, causing a CME.In practice, I only saw this pool running at most 1 task. I don't really understand what's going on, so it seems possible that this PR could actually not solve the problem.
I think a better solution would be for
PaperImplementations#modifyCommandTreesSafely
to block the thread until theCOMMAND_SENDING_POOL
has finished all its tasks. However, I didn't see any way to do this without controlling how tasks are submitted to the pool.Anyway, I am definitely not familiar with multithreaded code, so I have no idea if what I've done here is a 'good' solution. I'm not sure if I'm submitting tasks correctly or handling stuff like
InterruptedException
safely. Feedback is greatly appreciated.So far, I've tested this manually on Paper and Spigot 1.20.2. It seems to resolve all the issues mentioned in #494. I plan to test this on other versions as well.
TODO:[ ]
Test on real servers (Paper+Spigot)I tested it, and it didn't work. See #501 (comment).